Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for subsystem logging.properties #4962

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add support for subsystem logging.properties #4962

wants to merge 1 commit into from

Conversation

edewata
Copy link
Contributor

@edewata edewata commented Feb 20, 2025

The CMSEngine, ACMEEngine, and ESTEngine have been modified to support subsystem logging.properties. If the file exists, it can be used to configure the logging level in various libraries (e.g. RESTEasy) to help troubleshooting.

The CA, ACME, and EST tests have been modified to create the subsystem logging.properties then check the debug log.

https://github.com/dogtagpki/pki/wiki/Configuring-Subsystem-Debug-Log

If RESTEasy logging is enabled, it will show something like this:

2025-02-20 17:06:11 [main] INFO: RESTEASY002225: Deploying javax.ws.rs.core.Application: class org.dogtagpki.acme.server.ACMEApplication
2025-02-20 17:06:11 [main] INFO: RESTEASY002200: Adding class resource org.dogtagpki.acme.server.ACMELoginService from Application class org.dogtagpki.acme.server.ACMEApplication
2025-02-20 17:06:11 [main] INFO: RESTEASY002200: Adding class resource org.dogtagpki.acme.server.ACMELogoutService from Application class org.dogtagpki.acme.server.ACMEApplication
2025-02-20 17:06:11 [main] INFO: RESTEASY002200: Adding class resource org.dogtagpki.acme.server.ACMEEnableService from Application class org.dogtagpki.acme.server.ACMEApplication
...

@edewata
Add support for subsystem logging.properties
2c1ab9e 
The CMSEngine, ACMEEngine, and ESTEngine have been modified
to support subsystem logging.properties. If the file exists,
it can be used to configure the logging level in various
libraries (e.g. RESTEasy) to help troubleshooting.

The CA, ACME, and EST tests have been modified to create
the subsystem logging.properties then check the debug log.

https://github.com/dogtagpki/pki/wiki/Configuring-Subsystem-Debug-Log
@edewata edewata requested review from fmarco76 and jmagne February 20, 2025 17:20
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
64.2% Duplication on New Code

See analysis details on SonarQube Cloud

fmarco76
fmarco76 reviewed Feb 20, 2025
View reviewed changes
Copy link
Member

@fmarco76 fmarco76 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For testing purpose I think it is as quicker as modify the logging.properties in the webapp.

If this is a was to allow different customisation in case of multiple instances running in the same server then we should provide a full customisation of the logger.

base/acme/src/main/java/org/dogtagpki/acme/server/ACMEEngine.java
String value = properties.getProperty(key);

logger.info("- " + key + ": " + value);
if (!key.endsWith(".level")) continue;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we call the file logging.properties I am expecting that the provided values should overwrite the existing to modify the logs. So I would try to update the full configuration

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I can try that. I wasn't sure whether the LogManager will override just the params specified in the custom logging.properties or replace the entire logging config with the custom logging.properties (meaning the handlers need to be redefined in the custom logging.properties).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried this and it didn't work since Tomcat is using ClassLoaderLogManager which does not implement updateConfiguration() and the base implementation in LogManager doesn't support Tomcat's hierarchical class loader.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess there was no way to have one copy of this?

base/est/src/main/java/org/dogtagpki/est/ESTEngine.java

logger.info("- " + key + ": " + value);
if (!key.endsWith(".level")) continue;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above

base/server/src/main/java/com/netscape/cmscore/apps/CMSEngine.java

logger.info("CMSEngine: - " + key + ": " + value);
if (!key.endsWith(".level")) continue;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above

@edewata
Copy link
Contributor Author

edewata commented Feb 20, 2025

For testing purpose I think it is as quicker as modify the logging.properties in the webapp.

Yes, for testing we can just modify the default logging.properties but the changes are temporary (will be lost after upgrade).

If this is a was to allow different customisation in case of multiple instances running in the same server then we should provide a full customisation of the logger.

Even with a single instance, the custom logging.properties can be used to permanently customize the logging config (e.g. only log error messages) just like setting the debug.level in CS.cfg which won't be affected by upgrade. Let me try the LogManager as you suggested. Thanks!

fmarco76
fmarco76 approved these changes Feb 24, 2025
View reviewed changes
Copy link
Member

@fmarco76 fmarco76 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK!. Since update is not supported in tomcat thelogging.properties has a limited use but it might be useful for debugging. I am accepting this PR.

@edewata
Copy link
Contributor Author

edewata commented Feb 24, 2025

As mentioned above, the LogManager.updateConfiguration() doesn't work in Tomcat. So for now I'll keep the PR as is which can only override the logging level. This is sufficient and also necessary for configuring the logging level in ACME and EST containers since they don't have CS.cfg and changes to the default logging.properties will be lost when the containers are restarted. If we need additional logging customization we can deal with that separately later.

@edewata
Copy link
Contributor Author

edewata commented Feb 24, 2025

@fmarco76 Thanks!

jmagne
jmagne approved these changes Feb 26, 2025
View reviewed changes
Copy link
Contributor

@jmagne jmagne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good after taking in consideration marco's concerns. I would imagine one would use this in extreme debugging situations trying to dig out info. My one question was if the routine used in 3 or 4 places to get the logging file could be made common. No big deal though. Approving.

base/acme/src/main/java/org/dogtagpki/acme/server/ACMEEngine.java
String value = properties.getProperty(key);

logger.info("- " + key + ": " + value);
if (!key.endsWith(".level")) continue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess there was no way to have one copy of this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmarco76 fmarco76 fmarco76 approved these changes

@jmagne jmagne jmagne approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@edewata @fmarco76 @jmagne